[fix](storage) align pruned complex column types with sub-column iterators on merge/agg read paths#65396
Open
csun5285 wants to merge 1 commit into
Open
[fix](storage) align pruned complex column types with sub-column iterators on merge/agg read paths#65396csun5285 wants to merge 1 commit into
csun5285 wants to merge 1 commit into
Conversation
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: requesting changes for one regression-test oracle issue.
Critical checkpoint conclusions:
- Goal/correctness: the C++ change threads pruned complex-column data types through the query scan schema, merge iterator, and aggregate reader paths; I did not find an additional functional bug in those inspected paths.
- Scope: the implementation is focused on query-pruned type propagation plus one regression test.
- Concurrency/lifecycle: no new thread, lock, static initialization, or ownership lifecycle risk was introduced by the changed code.
- Compatibility/persistence/config: no new persisted format, FE-BE protocol field, or config item was added; the pruned-column type map is query-local and
shawdow_copy_without_columnsclears it. - Parallel paths: TopN merge-reader, aggregate reader, ordinary materialization/rowid fetch, and compaction/vertical-reader paths were checked. Compaction/vertical paths do not use query-pruned maps.
- Tests: the regression targets the right failure family, but its layout self-check does not prove the rowset has more than one segment, which is required to select
VMergeIterator. - Validation: static review only. This checkout is shallow/missing the base SHA,
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc, so I did not build or run regression tests. - User focus: no additional user-provided review focus was present.
Subagent conclusions: optimizer-rewrite reported no valuable findings. tests-session-config proposed TSC-001, accepted as MAIN-001 and submitted inline. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
4 times, most recently
from
6141226 to
7cad469
Compare
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
4 times, most recently
from
d69fffb to
ac507fb
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
…ators on merge/agg read paths
When a query prunes nested sub-columns of a complex column (EXPLAIN shows
"pruned type"), the scan's struct sub-column iterators are filtered down to
the pruned subset (set_access_paths + remove_pruned_sub_iterators), while the
FE-pruned data type is recorded only in TabletSchema::_pruned_columns_data_type
and honored only by TabletSchema::create_block*. Read paths that materialize
column shapes from other sources still used the FULL storage type, so the
positional pairing in StructFileColumnIterator::next_batch worked on two
different ordinal spaces:
1. VMergeIteratorContext::block_reset built per-segment merge blocks from the
storage Schema (full type). A topn ordered read over a rowset with multiple
OVERLAPPING segments then decoded pruned children into the wrong dst
columns:
- "[E-3110] Method insert_many_dict_data is not supported for TINYINT"
(dict-encoded text child decoded into a tinyint child)
- "... is not supported for INT" (single-child pruning variant)
- BE SIGSEGV when the shifted child types happen to match and the loop
runs past the pruned iterator vector (same family as DORIS-26595)
This is the root cause of DORIS-26889: the CI's s3 insert naturally
produces overlapping multi-segment rowsets, while single-segment local
loads never take the merge path. On master the default plan usually
shields this path via TopN lazy materialization (apache#63736 + apache#64242), but
e.g. raising topn_opt_limit_threshold, disabling
topn_lazy_materialization_threshold, or light_schema_change=false tables
expose it; branch-4.1 hits it by default.
2. BlockReader::_init_agg_state built the reader_replace argument type from
the raw TabletColumn (full type) while the stored block uses the pruned
type. On AGG tables any pruned read that goes through the aggregate merge
path (>= 2 data rowsets, no special session vars needed) failed with
"Aggregate function reader_replace argument 0 type check failed:
ColumnStruct has 2 elements, but DataTypeStruct has 3 elements".
Fix: make the shape-materializing read paths use the same FE-pruned type as
the sub-column iterators, so dst blocks and iterators stay in one ordinal
space by construction.
- VMergeIteratorContext::init reads the FE-pruned complex types from
StorageReadOptions::tablet_schema (TabletSchema::pruned_columns_data_type(),
already set on the query read path), and block_reset then builds each merge
block with the pruned type for pruned columns and the full storage type for
the rest, so the block's struct children align with the pruned sub-column
iterators. No new plumbing through the merge iterator or the storage Schema.
- TabletColumn::get_aggregate_function now takes the column's actual data type
explicitly. BlockReader::_init_agg_state passes the stored block's column
type (already the pruned type for pruned complex columns), while the
load/compaction/schema-change paths pass the full column type.
The new regression case constructs an OVERLAPPING multi-segment rowset
deterministically (MemTable.need_flush debug point + NULL keys in every
batch, segment count kept below the segcompaction threshold), self-checks the
layout via tablet meta, and covers all variants: the dict-error and
SIGSEGV topn shapes on the inline path, the AGG default-session shape across
two rowsets, and the VMaterializeNode default path as a guard. All variants
fail before this fix and pass after it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
csun5285
force-pushed
the
fix-topn-pruned-struct-merge
branch
from
ac507fb to
fb7a8df
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no new distinct inline comments in this pass. The existing line-100 thread remains the only actionable issue I found, and I did not duplicate it.
Critical checkpoint conclusions:
- Goal/correctness: the PR aligns FE-pruned complex-column types with the merge block and aggregate-reader function paths. I checked the scan setup,
TabletReader,BetaRowsetReader,VMergeIteratorContext,BlockReader::_init_agg_state, rowid-fetch materialization, and compaction/schema-change call sites; I found no additional correctness issue. - Scope: the implementation is focused on pruned type propagation plus one regression test.
- Concurrency/lifecycle: no new thread, lock, static initialization, or storage lifetime risk was introduced; the pruned type map is query-local and read through the tablet schema held by the reader path.
- Compatibility/config/persistence: no new persisted format, FE-BE protocol field, or config item was added.
- Parallel paths: query TopN merge and AGG reader paths use the pruned block/type shape; load, schema-change, and vertical compaction callers continue to pass full storage types.
- Tests: the regression covers default materialization, forced inline TopN, and AGG merge reads. The remaining test oracle gap is already covered by existing thread discussion_r3544580724: line 100 still checks only
segments_overlap_pband should also assertnum_segments > 1. - Validation: static review only. This checkout is shallow and lacks
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc, so I did not build or run regression tests. I refreshed the live PR head (fb7a8dfb66da7c750db6101d2dd6c2bb38331e90); it differs from the local checkout only by comment wording in two C++ files. - User focus: no additional user-provided review focus was present.
Subagent conclusions: both required subagents reported only the duplicate of existing thread discussion_r3544580724. The main merge recorded that as MAIN-DUP-001 with no new proposed inline comments. Convergence round 2, after the live-head refresh, ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same ledger/comment set.
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
TPC-H: Total hot run time: 29975 ms |
Contributor
TPC-DS: Total hot run time: 181241 ms |
Contributor
ClickBench: Total hot run time: 25.11 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VMergeIteratorContext::block_reset built per-segment merge blocks from the storage Schema (full type). A topn ordered read over a rowset with multiple OVERLAPPING segments then decoded pruned children into the wrong dst columns
BlockReader::_init_agg_state built the reader_replace argument type from the raw TabletColumn (full type) while the stored block uses the pruned type. On AGG tables any pruned read that goes through the aggregate merge path (>= 2 data rowsets, no special session vars needed) failed with "Aggregate function reader_replace argument 0 type check failed: ColumnStruct has 2 elements, but DataTypeStruct has 3 elements".
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)